feat(rpc): simplify SyncChainMmr endpoint#2069
Conversation
This endpoint is now always syncing up to the committed tip of the chain. Some extra data is also returned, like the latest proven block number and the MMR path of the committed tip.
|
Without having looked at the code, a couple of questions/comments:
I guess the alternative shape of this endpoint could be: message SyncChainMmrRequest {
fixed32 current_block_height = 1;
FinalityLevel finality_level = 2;
}
enum FinalityLevel {
FINALITY_UNSPECIFIED = 0;
FINALITY_COMMITTED = 1;
FINALITY_PROVEN = 2;
}This is not as big of a change compared to the current endpoint - and so, maybe it doesn't simplify things too much. But it does feel a bit more flexible and easy to extend in the future. |
I think this would still be a simplification (removing the arbitrary block number target). I'm fine with this one too, we should just finalize this. That would leave us with the following: enum FinalityLevel {
FINALITY_UNSPECIFIED = 0;
FINALITY_COMMITTED = 1;
FINALITY_PROVEN = 2;
}
message SyncChainMmrRequest {
fixed32 current_block_height = 1;
FinalityLevel finality_level = 2;
}
message SyncChainMmrResponse {
BlockRange block_range = 1;
primitives.MmrDelta mmr_delta = 2;
blockchain.BlockHeader block_header = 3;
}
|
|
Catching up on this.. iiuc
They would still be called the same thing, just coincidentally be identical. Or we could update the docs to state they're identical and have it be official. No API change required. tbh I would have separated/normalized by removing the block header etc information completely. enum HeaderRequest {
BlockNumber(u32),
CommittedTip,
ProvenTip,
L1FinalizedTip,
}But I know preference so far has been fewer but broader requests.. --edit-- Okay so this only enables a |
|
OK, let's converge on option 1 then. I'll update the proposal in the original bug. |
I don't think the user would be asking for specific block height (and maybe that's what you ment in the edit.
Only on MMR will be sufficient, but the client will need to make 2 requests:
Both requests update the same MMR on the client, but now the MMR contains enough info to execute transactions against either proven or committed blocks.
Right - and i'm fine keeping it as Separately, following my comment from https://github.com/0xMiden/miden-client/pull/1887/changes#r3236867263, I think we may need to include the block signature in the response as well (it is not a part of the block header AFAICT). So, the response my look more like: message SyncChainMmrResponse {
BlockRange block_range = 1;
primitives.MmrDelta mmr_delta = 2;
blockchain.BlockHeader block_header = 3;
blockchain.BlockSignature validator_signature = 4;
} |
|
Based on the discussion here I'm closing this in favor of #2075. |
Miden client uses the
SyncChainMmrendpoint exclusively to sync up to the committed tip of the chain. This PR simplifies the request so that it no longer supports arbitrary block ranges. The client is now expected to include the block number of its latest known block in the request and the response always contains the delta up to the committed tip.The new interface looks like this:
Extra fields added are:
latest_proven_block_num: now we always include the block number of the proven blocklatest_committed_mmr_pathis the MMR path of the committed tipOther changes:
latest_committed_mmr_deltaandlatest_committed_mmr_pathare only present in the response if the delta is non-empty (ie.current_block_height != latest_committed_header)Closes #2044